-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add more states to pending activift info #457
Conversation
@@ -106,6 +106,8 @@ enum PendingActivityState { | |||
PENDING_ACTIVITY_STATE_SCHEDULED = 1; | |||
PENDING_ACTIVITY_STATE_STARTED = 2; | |||
PENDING_ACTIVITY_STATE_CANCEL_REQUESTED = 3; | |||
PENDING_ACTIVITY_STATE_BACKING_OFF = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in our slack conversation. This is an artificial state and the state machine doesn't record this explicitly to save DB updates. I think it's nice to be able to surface this state to users though so I support adding it to the API.
@@ -106,6 +106,8 @@ enum PendingActivityState { | |||
PENDING_ACTIVITY_STATE_SCHEDULED = 1; | |||
PENDING_ACTIVITY_STATE_STARTED = 2; | |||
PENDING_ACTIVITY_STATE_CANCEL_REQUESTED = 3; | |||
PENDING_ACTIVITY_STATE_BACKING_OFF = 4; | |||
PENDING_ACTIVITY_STATE_PAUSED = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in another PR. I wouldn't model this as a distinct state and instead have another dimension for expressing pause status to be able to represent scenarios such as: an attempt is currently started and the activity has been requested to pause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From user perspective I don't see a scenario when it matters from what state activity comes to the PAUSE state.
Activity cannot be started and paused, it is just paused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. These states are more where the activity is at in general (started, scheduled, etc) and not specific things about the already-started activity. Would like to see more specific details on the pending activity itself (and really, the describe activity and pending activity should have the same information, ideally even the same object).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure what you agree with @cretz.
FTR the scheduler state has a separate bool for paused:
api/temporal/api/schedule/v1/message.proto
Line 281 in c038641
bool paused = 2; |
I would rather have this consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I see my message posted at the same time as @ychebotarev.
I agree with the original statement that should not be in the enumerate for activity state. A paused or backing off activity is still "scheduled" or "started" based on how this enum is used. Not only is it a backwards incompatible change to suddenly start making backing-off activities be a different enum, but I think whether an activity is paused or backing-off is unrelated to this state which is more of an overall one of whether it has ever started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cretz - there will be bool variable in PendingActivityInfo that indicates that activity is paused. PendingActivityInfo will also be part of the DescribeActivity response. This state is a separate thing. It is more about "give it quick look and understand where are we".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"started" is not "running" and to make people's code that expects backing-off activities to be this new enum instead of what it always was is backwards incompatible. If we need a more detailed value somewhere because "started" needs to be split into multiple values, that makes sense. IMO a paused activity can be scheduled/started, as can a backing off activity (and is how people have coded against it today).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @ychebotarev offline. I'm okay with the way he decided to represent these states and he's clarified that internally we don't lose any information.
What changed?
Add
PENDING_ACTIVITY_STATE_BACKING_OFF = 4;
PENDING_ACTIVITY_STATE_PAUSED = 5;
states.
Why?
Users need to differentiate between 'waiting for retry' and 'waiting to be started' stated.
Also need an explicit 'PAUSED' state
Breaking changes
It is not a breaking change